-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add custom OpenTelemetrySpanExt trait #700
Add custom OpenTelemetrySpanExt trait #700
Conversation
crates/utils/src/tracing/mod.rs
Outdated
pub use opentelemetry::trace::Status as OtelStatus; | ||
pub use tracing_opentelemetry::OpenTelemetrySpanExt; | ||
use opentelemetry::trace::Status as OtelStatus; | ||
use opentelemetry::{Key, Value}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mirko-von-Leipzig in the issue you mentioned:
We can make set_attribute more ergonomic by adding a secondary trait ToValue which we can then implement for foreign types. i.e. instead of set_attribute("hash", digest.to_hex()), we can just do set_attribute("hash", &digest).
I was wondering if this is worthwhile considering that crates can already impl From<...> for opentelemetry::Value
for their types and rely on the trait as is , value: impl Into<Value>);
.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of our core types come from the crates from miden-base
- and I don't think introducing open-telemetry
there is a reasonable expectation. Though it could make sense if we really want to standardize on it everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually here we go: https://docs.rs/opentelemetry_sdk/latest/opentelemetry_sdk/testing/trace/index.html
They do actually have test infra :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking you meant that this is relevant for this PR and not the other trace-related PR #698.
TestSpan
implements trace::Span
so I would have to change our Ext trait to be over impl<S: trace::Span + tracing_opentelemetry::OpenTelemetrySpanExt>
and then make sure the tracing_opentelemetry::OpenTelemetrySpanExt
applies to TestSpan
. Unless I am missing something about how to using those testing utilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right; yeah this was intended for their 😅 Will cross-post just for traceability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Yes good point - we can impl<S: OpenTelemetrySpanExt>
because it in turn is implemented for T: tracing::Span
. So we just piggy back on theirs? Alternatively we keep as is and add another impl for the TestSpan
specifically.
I'd do the former, and then see how it works out in the other PR?
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
Closes #678
tracing_opentelemetry::OpenTelemetrySpanExt
with a custom trait of the same nameopentelemetry::trace::Status
in favour of a trait API that takes&dyn Error
insteadToValue
trait and impls for various Miden and core types